Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature/snet rewrite #811

Merged
merged 59 commits into from
Jul 8, 2021
Merged

Conversation

i-hate-nicknames
Copy link
Contributor

Rewriting network initialization to gain init performance, as well as cleaner code and better organization in snet and transport packages.

Did you run make format && make check?
not yet

Fixes #760

Changes:

How to test this PR:

Copy link
Member

@jdknives jdknives left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall good job. I think the naming renaming of certain structures and usage of dmsgC instead of generic snet structures is helpful. In general the refactor seems to go in the right direction and works. I have not checked if the performance is significantly changed but that is not very important anyways.

Two comments:

  1. I am not yet sure if there is more work to be done on structs we define to wrap connections yet. If I see correctly we have a type Conn interface and a type Client interface. The naming of Conn may be somewhat confusing as it is a wrapper for a net.Conn and maybe should be called Transport? I am also thinking about how to simplify each of these structures (including managed transport).

One of the ideas I had and discussed with Synth was to get rid of redialing and status logic as described in #830

Will have a look and see if there are further ways to reduce complexity in these interfaces and structures.

  1. All of the tests resembling integration tests should be torn our here and moved to the services repo. There is an open PR where we can just comment on with tests that should be redone there with docker setup.

pkg/snet/snettest/env.go Outdated Show resolved Hide resolved
pkg/visor/init.go Outdated Show resolved Hide resolved
@i-hate-nicknames i-hate-nicknames marked this pull request as ready for review July 3, 2021 11:08
@i-hate-nicknames i-hate-nicknames changed the title WIP Feature/snet rewrite Feature/snet rewrite Jul 3, 2021
@jdknives jdknives merged commit 4a13052 into skycoin:develop Jul 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants